Draft: Initial work on OpenFX module support #1051
Conversation
|
I will require that you fix some bugs before you can submit new features. It is not fair to other developers if a person only does the more fun work of features but never helps fixing bugs. |
That's fair. you have some bugs in mind? is it something related to my module (lv2, vst2) or in general? |
| filter_openfx.c | ||
| ) | ||
|
|
||
| file(GLOB YML "*.yml") |
There was a problem hiding this comment.
I'd rather be explicit about the YAML files, because I think there won't be that many services here.
|
|
||
| target_compile_options(mltopenfx PRIVATE ${MLT_COMPILE_OPTIONS}) | ||
|
|
||
| target_link_libraries(mltopenfx PRIVATE mlt m) |
There was a problem hiding this comment.
This assumes that every platform has libm though that might not be the case, just link it if it's needed.
|
|
||
| static const char *getArchStr() | ||
| { | ||
| if(sizeof(void *) == 4) { |
There was a problem hiding this comment.
I think that there's a precompiler definition out there that describes the architecture bits, look for it if there's one.
| MltOfxHost.host = (OfxPropertySetHandle) mlt_properties_new (); | ||
| mltofx_init_host_properties (MltOfxHost.host); | ||
|
|
||
| char *dir, *openfx_path = getenv("OFX_PLUGIN_PATH"); |
There was a problem hiding this comment.
Brilliant that you've thought ahead about multiple paths, though, is there a way to set a default?
| de = readdir(d); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
You are mixing tabulation characters here, please run clang-format or ninja astyle.
There was a problem hiding this comment.
You have lots of places that are yet to be worked on, and that's okay. I mean, this PR is meant to be the starting point. Albeit, I'd do a pair of things, comment TODO where appropriate to let know why the feature is incomplete and split suites into their own files to organize code better.
| propSetString ((OfxPropertySetHandle) clip_prop, kOfxImagePropField, 0, kOfxImageFieldNone); /* I'm not sure about this */ | ||
|
|
||
| char *tstr = calloc(1, strlen ("Output") + 11); | ||
| sprintf (tstr, "%s%04d%04d", "Output", rand () % 9999, rand () % 9999); /* WIP: do something better */ |
There was a problem hiding this comment.
There's a chance for collision if you use random numbers, but what about hashing? That's what Natron does.
| (OfxPropertySetHandle) get_roi_out_args); | ||
| mltofx_log_status_code (status_code, kOfxImageEffectActionGetRegionsOfInterest); | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit, this should come after begin sequence render per the header declaration and what end by itself means.
| void | ||
| mltofx_set_source_clip_data (OfxPlugin *plugin, mlt_properties image_effect, uint8_t *image, int width, int height); | ||
|
|
||
| void | ||
| mltofx_set_output_clip_data (OfxPlugin *plugin, mlt_properties image_effect, uint8_t *image, int width, int height); |
There was a problem hiding this comment.
Either name the first as mltofx_set_input_clip_data or the second as mltofx_set_destination_clip_data.
|
|
||
| mltofx_log_status_code (status_code, kOfxImageEffectActionRender); | ||
| } | ||
|
|
There was a problem hiding this comment.
This should go after the next one per what's declared in the header.
Since I reached presentable state I published this change so I can receive feedback from the community.
This video shows testing the module with the
uk.co.thefoundry.OfxInvertExampleopenfx sample plugin:openfxtest.mp4
Yes this implementation still lack many things such as opengl, draw suit (which is something the plugins could use for drawing vector graphics like post script or cairo), multi-threading, openfx logging extension and still many things ignored and implemented as dummy functions.
Also this is still under testing it crashes for my many times.